Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

make inplace Rational{BigInt} arithmetic from gmplib available #44566

Merged
merged 15 commits into from
Apr 14, 2022
Merged

make inplace Rational{BigInt} arithmetic from gmplib available #44566

merged 15 commits into from
Apr 14, 2022

Conversation

sumiya11
Copy link
Contributor

Hi ! This pr restructures arithmetic operations on Rational{BigInt} in gmp.jl to make inplace GMP for big rationals (add!, sub!, mul!, and div!) available from Base.GMP.MPQ module.

There it also specializes cmp (gmp function described in here https://gmplib.org/manual/Comparing-Rationals as mpz_cmp), that turns out to be faster than current version in Base.

Current version of cmp

x = Rational{BigInt}(12345678, 123)
y = Rational{BigInt}(12345679, 123)
@btime cmp($x, $y) 
  323.478 ns (6 allocations: 96 bytes) 

with cmp from GMP :

x = Rational{BigInt}(12345678, 123)
y = Rational{BigInt}(12345679, 123)
@btime cmp($x, $y)
  48.283 ns (2 allocations: 128 bytes)

@Liozou
Copy link
Member

Liozou commented Mar 11, 2022

It looks like you did not touch the code paths handling rational infinity, like big(1//0). It does not seem to affect the usual +, -, * and / operations but the inplace versions are affected: for instance

julia> a = big(1//0);

julia> b = big(0//1);

julia> Base.GMP.MPQ.sub!(b, a);

julia> b
0//1

when I'd expect it to be -1//0.
Those inplace functions are not exported nor documented and it has no impact on usual operations so I don't know if it's a blocker, but it feels a bit dangerous.

@giordano giordano added rationals The Rational type and values thereof bignums BigInt and BigFloat labels Mar 11, 2022
@sumiya11
Copy link
Contributor Author

@Liozou thanks for your comment! I fixed inplace for infinities, and added some tests

base/gmp.jl Show resolved Hide resolved
base/gmp.jl Outdated Show resolved Hide resolved
base/gmp.jl Outdated Show resolved Hide resolved
base/gmp.jl Outdated Show resolved Hide resolved
base/gmp.jl Show resolved Hide resolved
base/gmp.jl Outdated Show resolved Hide resolved
sumiya11 and others added 4 commits March 12, 2022 20:28
Co-authored-by: Lionel Zoubritzky <[email protected]>
Co-authored-by: Lionel Zoubritzky <[email protected]>
Co-authored-by: Lionel Zoubritzky <[email protected]>
@sumiya11
Copy link
Contributor Author

sumiya11 commented Mar 12, 2022

Just a very simple benchmark of how much speed up is possible using preallocated Rational for result and inplace operations, (note that these are very noisy)

for x, y, z of Rational{BigInt}

+(x, y) *(x, y) add!(z, x, y) mul!(z, x, y)
480 ns 530 ns 150 ns 220 ns

for x, y of Int

unsafe_rational(BigInt, x, y) set_si!(z, x, y) set_z!(z, x) BigInt()
140 ns 20 ns 20 ns 60 ns
The benchmark code
using BenchmarkTools

z, x, y = Rational{BigInt}(0, 1), Rational{BigInt}(123456789, 3456789), Rational{BigInt}(2323223, 11991398)

@btime +($x, $y)

@btime *($x, $y)

@btime Base.GMP.MPQ.add!($z, $x, $y)

@btime Base.GMP.MPQ.mul!($z, $x, $y)

@info "###########"

x, y = 12345, 3423437

@btime Base.unsafe_rational(BigInt, $x, $y)

@btime Base.GMP.MPQ.set_si!($z, $x, $y)

x = BigInt(2123434)

@btime Base.GMP.MPQ.set_z!($z, $x)

base/gmp.jl Outdated Show resolved Hide resolved
base/gmp.jl Outdated Show resolved Hide resolved
test/gmp.jl Outdated Show resolved Hide resolved
test/gmp.jl Outdated Show resolved Hide resolved
test/gmp.jl Show resolved Hide resolved
test/gmp.jl Outdated Show resolved Hide resolved
test/gmp.jl Outdated Show resolved Hide resolved
test/gmp.jl Outdated Show resolved Hide resolved
sumiya11 and others added 7 commits March 16, 2022 16:18
Co-authored-by: Lionel Zoubritzky <[email protected]>
Co-authored-by: Lionel Zoubritzky <[email protected]>
Co-authored-by: Lionel Zoubritzky <[email protected]>
Co-authored-by: Lionel Zoubritzky <[email protected]>
Co-authored-by: Lionel Zoubritzky <[email protected]>
@sumiya11
Copy link
Contributor Author

@Liozou thank you a lot for your comments !

@sumiya11
Copy link
Contributor Author

Do you think the need to store rat::Rational{BigInt} in _MPQ can be relaxed ? If so, we can probably do

@eval $op(a::$T) = $op!(unsafe_rational(BigInt(), BigInt()), a)

more efficient by using set_si!(_MPQ(), 0, 1) directly instead of unsafe_rational

base/gmp.jl Outdated Show resolved Hide resolved
@Liozou
Copy link
Member

Liozou commented Mar 16, 2022

In the definition of _MPQ, the need for the rat field is indicated to be related to GC, which means (I assume) that otherwise the GC does not know that the pointers in the _MPQ struct refer to the BigInts in rat, so it may corrupt those structs. I'm no GC guru so maybe there is a way to avoid that, but I don't see it...

I think one way to completely bypass the issue consists in not using Rational{BigInt} but directly the gmpq object: for this, I direct you towards my BigRationals.jl library, which is a very basic wrapper around GMPQ. But that's orthogonal to this PR.

@Liozou
Copy link
Member

Liozou commented Mar 16, 2022

There is a more in-depth discussion about this in #9826

Copy link
Member

@Liozou Liozou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this PR! I think it's good now, and a nice addition.

@JeffBezanson JeffBezanson merged commit 02b6d99 into JuliaLang:master Apr 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bignums BigInt and BigFloat rationals The Rational type and values thereof
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants